Fix native AoT test scripts #1540
Conversation
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1540 +/- ##
==========================================
- Coverage 73.91% 73.37% -0.55%
==========================================
Files 267 256 -11
Lines 9615 9457 -158
==========================================
- Hits 7107 6939 -168
- Misses 2508 2518 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@Yun-Ting Could you also review? |
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
build/test-aot-compatibility.ps1
Outdated
| $rootDirectory = Split-Path $PSScriptRoot -Parent | ||
| $publishOutput = dotnet publish $rootDirectory/test/OpenTelemetry.AotCompatibility.TestApp/OpenTelemetry.AotCompatibility.TestApp.csproj -nodeReuse:false /p:UseSharedCompilation=false | ||
| $publishOutput = dotnet publish $rootDirectory/test/OpenTelemetry.AotCompatibility.TestApp/OpenTelemetry.AotCompatibility.TestApp.csproj --runtime $runtime -nodeReuse:false /p:UseSharedCompilation=false |
There was a problem hiding this comment.
You shouldn't need to pass --runtime in. It will be inferred on the platform this code is executing on.
There was a problem hiding this comment.
I was passing it explicitly so it married-up obviously with the output path, as the binary is found later down to run what was compiled, and I thought that made it easier to follow for the casual observer.
There was a problem hiding this comment.
it married-up obviously with the output path
Following that same logic you would be passing -f $targetNetFramework to publish, because we are using it in the output path. (not that I'm suggesting we actually do that as well).
I'm just of the opinion to not clutter up the command if it doesn't add value. It just adds more things for people to try to understand what this line is doing.
|
|
||
| $actualWarningCount = 0 | ||
|
|
||
| foreach ($line in $($publishOutput -split "`r`n")) | ||
| { | ||
| if ($line -like "*analysis warning IL*") | ||
| if (($line -like "*analysis warning IL*") -or ($line -like "*analysis error IL*")) |
There was a problem hiding this comment.
Under what cases does it emit analysis warning IL vs analysis error IL? Do you need to set "TreatWarningsAsErrors" for this to happen?
There was a problem hiding this comment.
To be honest, I'm not sure. I was working locally on a project I knew had AoT issues, but the script wasn't flagging any issues. When I manually ran the commands this script was doing, I found the errors.
There was a problem hiding this comment.
I can get dotnet publish for PublishAot=true to produce "analysis error IL" only on a 9.0 SDK and if TreatWarningsAsErrors=true. So this seems like the a valid change to make (especially if people are using the script as a starting point).
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
|
If the goal of the PR is to support running verifyaotcompat as one of the CI flow on Windows, the below part will need to be updated, too. That being said, I don't think we'll need to commit the change to the shell file if we are just trying to run the script locally : ). |
|
No, the goal was just to make the script re-usable locally on a Windows computer. I found that I couldn't use it while working on several PRs recently to add AoT support for some of the AWS libraries. People should be able to easily use it to lower the barrier to contribution. I can also update the test matrix to validate it on Windows as well if that's desired. |
|
"I can also update the test matrix to validate it on Windows as well if that's desired." -> yes, please. |
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
Support the AoT compatibility script being run locally on Windows.
.NET 8 supports native AoT publishing on macOS.
- Script did not fail for errors, only warnings. - Script did not fail if publishing failed.
If publish fails, show why.
Use `Push-Location` and `Pop-Location` to resolve PowerShell lint warnings in Visual Studio Code.
- Remove explicit `--runtime` flag. - Cater for macOS explicitly.
Add Windows to the matrix for verifying AoT support.
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
Changes
dotnet publishfails.Found while looking into an app of mine is getting trim warnings from OpenTelemetry.Instrumentation.AWS to see if they were easy to fix.